-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BREAK: remove spin projections in cons rules #282
BREAK: remove spin projections in cons rules #282
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
ingoing_spins: List[SpinEdgeInput], | ||
outgoing_spins: List[SpinEdgeInput], | ||
ingoing_spin_magnitudes: List[EdgeQN.spin_magnitude], | ||
outgoing_spin_magnitudes: List[EdgeQN.spin_magnitude], | ||
interaction_qns: SpinMagnitudeNodeInput, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something I realize now is that this rule seems to be checking two things and therefore needs
Maybe we should write an issue for this if this rule can be split up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also not really a suitable class name:
qrules/src/qrules/conservation_rules.py
Lines 467 to 470 in a045be5
class SpinMagnitudeNodeInput: | |
l_magnitude: NodeQN.l_magnitude = field(converter=NodeQN.l_magnitude) | |
s_magnitude: NodeQN.s_magnitude = field(converter=NodeQN.s_magnitude) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah seems fishy to me. @grayson-helmholz could you post an issue to split this rule into spin magnitude and
angular_momentum=Spin(ang_mom_mag, 0), | ||
coupled_spin=Spin(coupled_spin_mag, -1), | ||
), | ||
([1], [spin2_mag, 1], SpinNodeInput(ang_mom_mag, 0, coupled_spin_mag, -1)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of hard to read this test, but it was already like that unfortunately
This branch is currently being tested with AmpForm here: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🍰
Closes #280